[feature/patina-boot] patina_boot: Add discover_boot_options helper#1447
[feature/patina-boot] patina_boot: Add discover_boot_options helper#1447kat-perez wants to merge 8 commits intoOpenDevicePartnership:feature/patina-bootfrom
Conversation
✅ QEMU Validation PassedAll QEMU validation jobs completed successfully.
Workflow run: https://github.com/OpenDevicePartnership/patina/actions/runs/23876918302 Boot Time to EFI Shell
Dependencies
This comment was automatically generated by the Patina QEMU PR Validation Post workflow. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
8cee855 to
1892744
Compare
…DevicePartnership#1225) Adds BootOrchestration component, simple console discovery, simple BootOption Config - [x] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [x] Includes tests? - [x] Includes documentation? QEMU Platform Integration: - Q35 - SBSA N/A
… boot options (OpenDevicePartnership#1272) ## Description Add hotkey detection support to the boot orchestrator, allowing platforms to configure alternate boot options that are used when a hotkey (e.g., F12) is pressed during boot. Changes: - Add `detect_hotkey()` helper function to check for hotkey press via SimpleTextInput protocol - Add `hotkey_devices` field and `with_hotkey_device()` builder to `BootOptions` - Update `BootOrchestrator` to use alternate boot options when hotkey is detected --- - [x] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [x] Includes tests? - [ ] Includes documentation? ## How This Was Tested - Unit tests for `detect_hotkey()` (no input handles case) - Unit tests for `hotkey_devices` config (single device, multiple devices, combined with hotkey) - `cargo test -p patina_boot` passes ## Integration Instructions Platforms can configure hotkey boot options: ```rust BootOptions::new() .with_device(primary_device) .with_hotkey(0x16) // F12 scancode .with_hotkey_device(alternate_device) ``` Closes OpenDevicePartnership#1228
…e_system_table (OpenDevicePartnership#1284) Release SYSTEM_TABLE lock (TPL_NOTIFY) before accessing ComponentDispatcher (TPL_APPLICATION) to avoid TPL violation. PR OpenDevicePartnership#1225 lowered ComponentDispatcher from TPL_NOTIFY to TPL_APPLICATION to allow components to use boot services, but this created a conflict when initialize_system_table held SYSTEM_TABLE while setting boot/runtime services on ComponentDispatcher. Fix: Extract boot/runtime services pointers while holding SYSTEM_TABLE lock, then release it before accessing ComponentDispatcher. - [x] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? - Unit tests pass - QEMU Q35 boots without TPL violation panic N/A
…ion (OpenDevicePartnership#1290) Add support for expanding partial (short-form) device paths to full device paths by matching against the device topology. - Add `is_partial_device_path()` to detect partial paths (start with Media/Messaging nodes instead of Hardware/ACPI) - Add `expand_device_path()` to find matching full paths by enumerating device handles - Wire expansion into `boot_from_device_path()` for transparent handling - Currently supports HardDrive nodes with GPT partition signature matching This enables booting from Boot#### variables containing partial device paths like `HD(1,GPT,<GUID>)\EFI\BOOT\BOOTX64.EFI`. - [x] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [x] Includes tests? - [ ] Includes documentation? - Unit tests for `is_partial_device_path()`, `expand_device_path()`, and signature matching - `cargo test -p patina_boot` passes (32 tests) - QEMU Q35 platform test passes N/A Closes OpenDevicePartnership#1280
…stration design (OpenDevicePartnership#1333) ## Description Refactors `patina_boot` from a monolithic `BootOrchestrator` component + `Config<BootOptions>` pattern to a trait-based design: - **`BootOrchestrator` trait** — defines the boot flow interface with `execute() -> Result<!, EfiError>`, enforcing at the type level that successful boot never returns - **`BootDispatcher`** — the Patina component that installs the BDS architectural protocol and delegates to a `BootOrchestrator` implementation - **`SimpleBootManager`** — a default `BootOrchestrator` implementation for platforms with straightforward boot topologies - **`BootConfig`** — unified boot configuration (replaces previous `BootOptions` + `SimpleBootConfig` split), requires at least one device at construction (compile-time enforcement) Also updates `helpers.rs` imports from removed `uefi_protocol::device_path` to `device_path::paths`/`device_path::node_defs`. `patina_dxe_core` changes (image handle plumbing) split out to OpenDevicePartnership#1374. - [x] Impacts functionality? - [ ] Impacts security? - [x] Breaking change? - [x] Includes tests? - [ ] Includes documentation? ## How This Was Tested 1. Unit tests: 35 pass (`cargo test -p patina_boot`) 2. Integration tested on QEMU Q35 — all components dispatched, BDS phase ran, boot options attempted, failure handler fired correctly 3. CI: fmt, clippy, all platforms pass ## Integration Instructions Update boot orchestration usage from: ```rust use patina_boot::{component::BootOrchestrator, config::BootOptions}; // In configs(): add.config(BootOptions::new()...); // In components(): add.component(BootOrchestrator); ``` To: ```rust use patina_boot::{BootDispatcher, SimpleBootManager, config::BootConfig}; add.component(BootDispatcher::new(SimpleBootManager::new( BootConfig::new(primary_device_path()) .with_device(fallback_device_path()) .with_hotkey(0x16) .with_hotkey_device(usb_device_path()) .with_failure_handler(|| { /* ... */ }), ))); ```
…penDevicePartnership#1375) ## Description Rewrite `discover_console_devices()` from a stub into a full implementation that enumerates console protocol handles, builds multi-instance device paths, and writes `ConIn`, `ConOut`, and `ErrOut` UEFI global variables via `SetVariable`. - Adds `EFI_GLOBAL_VARIABLE` GUID to `patina::guids` - Adds `build_multi_instance_device_path()` helper for constructing multi-instance device paths from protocol handles - Updates `is_partial_device_path()` to recognize FV/FvFile paths as non-partial - Includes get_variable readback verification with device path display logging Depends on OpenDevicePartnership#1333. Closes OpenDevicePartnership#1230 - [x] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested Verified on QEMU Q35 with VGA enabled. Console variables written and read back successfully: - ConIn: 24 bytes (SimpleTextInput) - ConOut: 60 bytes (SimpleTextOutput + GOP) - ErrOut: 30 bytes (SimpleTextOutput) ## Integration Instructions N/A
…with DxeServices (OpenDevicePartnership#1422) ## Description Interleave controller connection with DXE driver dispatch during device enumeration. Connecting controllers can discover new firmware volumes (e.g., PCI option ROMs) that contain drivers for devices behind that controller. Those drivers must be dispatched before the next round of enumeration, otherwise the devices they serve will not be found. `SimpleBootManager` uses `interleave_connect_and_dispatch()` to alternate between connecting controllers and dispatching newly-discovered drivers until both stabilize. The `DxeDispatch` service trait (from OpenDevicePartnership#1421) is consumed via dependency injection. Note: `interleave_connect_and_dispatch()` currently uses `connect_all()` which connects every controller on every round. This is functional but inefficient for platforms with large device topologies — a future optimization could connect only newly-discovered controllers. - [x] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [x] Includes tests? - [ ] Includes documentation? ## How This Was Tested - Built SBSA DXE core binary with `BootDispatcher` + `SimpleBootManager` replacing TianoCore BdsDxe - Booted Windows ARM64 under QEMU SBSA-ref emulation with Patina BDS handling the full boot flow - Verified interleaving: controller connection discovered AHCI device, partial device path expanded to full path, Windows bootloader loaded, ExitBootServices completed ## Integration Instructions Depends on OpenDevicePartnership#1421 (`DxeDispatch` service trait) for platform binary integration. Remove TianoCore `BdsDxe.inf` from platform DSC/FDF since the `BootDispatcher` provides the BDS architectural protocol.
f168dff to
d9458cb
Compare
1892744 to
f39c078
Compare
| const LOAD_OPTION_ACTIVE: u32 = 0x00000001; | ||
|
|
||
| /// Null-terminated UTF-16 `BootOrder` variable name. | ||
| const BOOT_ORDER_VARIABLE_NAME: &[u16] = |
There was a problem hiding this comment.
I missed this on earlier ones, but for these various UEFI spec defined things, we should be taking them to r-efi as well. Hosting them here until they are merged there is fine.
| name.push(c as u16); | ||
| } | ||
| for c in hex_chars { | ||
| name.push(c.to_ascii_uppercase() as u16); |
There was a problem hiding this comment.
Aren't these all digits? What does to_ascii_uppercase do on a digit?
There was a problem hiding this comment.
char::from_digit(n, 16) returns lowercase hex and UEFI variable names use uppercase. The to_ascii_uppercase was there to fix the casing. I changed this to use format!("Boot{:04X}", option_number) to specify more clearly that we want uppercase hex.
| return None; | ||
| } | ||
|
|
||
| let attributes = u32::from_le_bytes([data[0], data[1], data[2], data[3]]); |
There was a problem hiding this comment.
Lots of from_le_bytes throughout all these changes. A place to use zerocopy?
There was a problem hiding this comment.
Updated to use zerocopy
f39c078 to
97c8715
Compare
Reads BootOrder and Boot#### UEFI variables to build a BootConfig from UEFI-compliant boot options, enabling SimpleBootManager to use standard boot variables instead of platform-hardcoded device paths.
97c8715 to
5428475
Compare
d9458cb to
f808a3e
Compare
| // Connect each handle recursively | ||
| for &handle in handles.iter() { | ||
| // SAFETY: Empty driver handle list and null device path are valid per UEFI spec | ||
| let _ = unsafe { boot_services.connect_controller(handle, Vec::new(), ptr::null_mut(), true) }; |
There was a problem hiding this comment.
It looks like this allocates a new vector for each handle which could be a lot of allocations, right? Do you know how many allocations (calls) are being made on QEMU?
| let boot_order_name: &[u16] = &[ | ||
| 'B' as u16, 'o' as u16, 'o' as u16, 't' as u16, 'O' as u16, 'r' as u16, 'd' as u16, 'e' as u16, 'r' as u16, 0, | ||
| ]; |
There was a problem hiding this comment.
You used encode_utf16() elsewhere in the file. Is there a reason not to use it here? Like:
let boot_order_name: Vec<u16> = "BootOrder\0".encode_utf16().collect();|
|
||
| let file_path_bytes = &rest[offset..file_path_end]; | ||
|
|
||
| // SAFETY: file_path_bytes points to a valid device path from the EFI_LOAD_OPTION. |
There was a problem hiding this comment.
My understanding is that this depends on the boot variable (e.g. Boot####) to have correctly sized device path nodes. For example, if a boot UEFI variable were written with an very large node length (possible since these are just non-authenticated UEFI variables), this call to try_from_ptr() will call from_raw_parts() which will use that length to read past the actual device path buffer.
Let me know if this is already checked somewhere, but, if not, I suggest that each device path node length is validated before handing the pointer to try_from_ptr().
| if let Err(e) = helpers::signal_ready_to_boot(boot_services) { | ||
| log::error!("signal_ready_to_boot failed: {:?}", e); | ||
| } |
There was a problem hiding this comment.
If I'm reading this right, it is just calling ready to boot once and then looping through boot options? If so, it is expected to be called per boot option attempt.
After all SysPrep#### variables have been launched and exited, the platform shall notify EFI_EVENT_GROUP_READY_TO_BOOT and EFI_EVENT_GROUP_AFTER_READY_TO_BOOT event groups. This should happen when the Boot Manager is about to load and execute Boot#### variables with Attributes set to LOAD_OPTION_CATEGORY_BOOT according to the order defined by BootOrder.
https://uefi.org/specs/UEFI/2.11/03_Boot_Manager.html#required-system-preparation-applications
Description
Add
discover_boot_options()helper topatina_boot::helpersthat reads UEFIBootOrderandBoot####variables to build aBootConfigfrom standard UEFI boot options.This enables any
BootOrchestratorimplementation that consumesBootConfigto use UEFI-compliant boot variables instead of requiring platforms to hardcode device paths. The function:BootOrderto determine boot attempt orderBoot####EFI_LOAD_OPTIONstructure to extract device pathsLOAD_OPTION_ACTIVE)BootConfigwith discovered devices in priority orderHow This Was Tested
feature/patina-booton QEMU Q35 — full boot to UEFI Shell 2.0Integration Instructions
Platforms can call
discover_boot_options()with runtime services to automatically populate aBootConfigfrom UEFI boot variables instead of constructing device paths manually. This works with anyBootOrchestratorimplementation that accepts aBootConfig: